Initial implementation of supervisor by kuqin12 · Pull Request #1385 · OpenDevicePartnership/patina

kuqin12 · 2026-03-11T00:32:33Z

Description

This is the initial implementation of MM supervisor and user core in Rust.

How This Was Tested

This was tested on QEMU Q35 platform and booted to OS desktop as well as passed supervisor test app.

Integration Instructions

The integration guide is listed in: https://github.com/kuqin12/mu_feature_mm_supv/blob/personal/kuqin/supv_init/SeaPkg/Docs/PlatformIntegration/PlatformIntegrationSteps.md#integraion-guide-for-rust-based-supervisor. Because it provides the implementation of MM supervisor init module.

patina-automation · 2026-03-11T00:32:42Z

⌛ QEMU Validation Pending

QEMU validation is pending on successful CI completion.

Note: Any previous results are available in this comment's edit history.

This comment was automatically generated by the Patina QEMU PR Validation workflow.

components/patina_mm/src/protocol/mm_supervisor_request.rs

kuqin12 · 2026-03-11T16:34:55Z

core/patina_internal_mm_common/src/lib.rs

+//! Shared type definitions for MM supervisor and user cores.
+//!
+//! This crate provides the communication structures and enumerations that define
+//! the ABI between the supervisor (ring 0) and user (ring 3) MM modules.


Change the folder name, it should be common to supv only

core/patina_internal_mm_alloc/src/lib.rs

Javagedes

I'm very concerned about the number of global statics in your implementation. I understand it may actually be necessary to be able to perform our SEA validation on the supervisor.

We need to consolidate the statics if possible. If we cannot (due to SEA validation requirements), then we need to make trait abstractions around each of the different statics so that at runtime they all link up, but for testing you can use mockall to mock the different statics. e.g. something like:

static MY_STATIC: SomeStatic = SomeStatic::new();

trait MyTrait {
    fn interface_fn1();
    fn interface_fn2();
}

impl MyTrait for SomeStatic {
    fn interface_fn1() { MY_STATIC.interface_fn1() }
    fn interface_fn2() { MY_STATIC.interface_fn2() }
}

This will allow us to use mockall with the trait to do proper mocking.

components/patina_mm/src/protocol/mm_supervisor_request.rs

Javagedes · 2026-03-11T16:29:56Z

patina_mm_user_core/src/pool_allocator.rs

+/// Implementors provide the actual page allocation mechanism. The supervisor
+/// implements this with direct SMRAM bitmap tracking; the user core implements
+/// this by issuing syscalls to the supervisor.
+pub trait PageAllocatorBackend: Send + Sync {


nit: I think this should just be called PageAllocator.

I also don't think you need this is_initialized function. Just let allocate_pages / free_pages return an PageAllocError with maybe a enum variation of NotReady / NotInitialized / etc.

patina_mm_user_core/src/pool_allocator.rs

Javagedes · 2026-03-11T16:33:11Z

core/patina_internal_mm_alloc/src/lib.rs

@@ -0,0 +1,335 @@
+//! Shared MM Pool Allocator


It really feels like we need to pull out some of the re-usable bits from the patina-dxe-core allocation functionality. I understand not all of that. But as much as possible.

The patian-dxe-core allocation may be useful. But the page allocator for DXE is too GCD centric, which is not a thing in MM. I thought about bringing in GCD, but gave up due to complicated logic and excessive TPL usage.

I briefly looked at the pool allocator from dxe environment. Admittedly, I was scared off by its boot services named statics... I probably need to revisit it.

This is fair. The GCD is very complex, especially if you don't need it.

You could still possibly use some fundamentals like the RBT / BST.

There are also some allocator crates you could use depending on your needs. Just hoping you don't need to completely re-invent the wheel :).

Yeah, I understand :) The original thought is that we will need to have something to get other functionalities going. I will create an issue on this topic so that we do not lose it.

Javagedes · 2026-03-11T16:42:54Z

patina_mm_user_core/src/pool_allocator.rs

+    ///
+    /// Protected by `lock`. The raw pointer is `!Send` but the outer struct
+    /// provides `Send + Sync` via the lock.
+    head: Mutex<*mut PoolBlockHeader>,


I don't have a great answer here as I'm just doing a skim / pass through this code, but I really think we need a better abstraction around the head here and all of the pointer work you are doing. Not necessarily for this PR but as a task.

Javagedes · 2026-03-11T18:14:52Z

patina_mm_user_core/src/protocol_db.rs

@@ -0,0 +1,92 @@
+//! Protocol/Handle Database


we do not want to carry two implementations of the protocol db. We need to abstract or allow configuration enough that the existing protocol db implementation meets your needs.

:) that's what I thought as well. then the tpl based lock got in the way and I took the shorter route. Like the allocator comment, we will do that once this stabilizes.

Javagedes · 2026-03-11T18:16:22Z

patina_mm_user_core/src/mm_dispatcher.rs

@@ -0,0 +1,378 @@
+//! MM Driver Dispatcher


We should not implement a whole new dispatcher. We should abstract and allow configurations enough that we can re-use the existing implementation.

Javagedes · 2026-03-11T18:17:56Z

core/patina_internal_cpu/src/save_state/mod.rs

Use the newer crate layout as mentioned earlier.

Javagedes · 2026-03-11T18:19:47Z

core/patina_internal_cpu/src/save_state/mod.rs

@@ -0,0 +1,397 @@
+//! SMRAM Save State Architecture Definitions


I don't see a service here. Are you sure this is the right location?

Javagedes · 2026-03-11T18:20:13Z

sdk/patina/src/timer/mod.rs

@@ -0,0 +1,135 @@
+//! Arch-specific timer functionality


This already exists does it not? I'm confused.

This does, but it was carried inside the dxe specific module. I am trying to get this into a common place. Any suggestions on where that should be? Probably not a component?

Will create a separate PR for this for better readability.

kuqin12 · 2026-03-17T04:24:26Z

patina_mm_supervisor_core/src/lib.rs

+#![cfg(target_arch = "x86_64")]
+#![feature(coverage_attribute)]
+
+#![allow(incomplete_features)]


This is for the generic_const_exprs so that the statics will be materialized based on the number of CPUs.

kuqin12 · 2026-03-17T04:26:29Z

patina_mm_supervisor_core/src/lib.rs

+    /// 
+    /// On the first call (initialization phase), this function returns after init is complete.
+    /// On subsequent calls, BSP enters the request loop and APs enter the holding pen (neither returns).
+    pub fn entry_point(&'static self, cpu_index: usize, hob_list: *const c_void) {


Agreed. This is a fully loaded monolithic flow. Will need to break up based on functionality here.

kuqin12 · 2026-03-17T04:28:51Z

patina_mm_supervisor_core/src/lib.rs

+                .ok_or(PolicyInitError::NullHobList)?
+        };
+
+        let mut supv_comm_buffer = 0 as u64;


Would you have better suggestions here? It is meant to collect the values and should be validated once they are all collected. The validation may not be there yet, but that is in the plan.

kuqin12 · 2026-03-17T04:38:36Z

patina_mm_supervisor_core/src/lib.rs

+        }
+
+        // Store communication buffer configuration.
+        COMM_BUFFER_CONFIG.call_once(|| CommBufferConfig {


This is a one-time initialization by taking the prepared buffer from HOB value. Then following MMIs will consume them rather than always going through the preparation. So in that sense I do want to cache these values, and once only. Any suggestion on how to avoid statics here?

kuqin12 · 2026-03-17T04:44:18Z

patina_mm_supervisor_core/src/mailbox.rs

+
+/// A single AP's mailbox for communication with the BSP.
+#[repr(C, align(64))] // Cache-line aligned to avoid false sharing
+pub struct ApMailbox {


C side has much more complicated data structure to host these functionalities. But I cut the functionalities some and only kept the atomic part. But I agree, this should not be structured to be C like layout.

kuqin12 · 2026-03-17T04:57:16Z

patina_mm_supervisor_core/src/supervisor_handlers.rs

+// GUID for gEfiDxeMmReadyToLockProtocolGuid
+// { 0x60ff8964, 0xe906, 0x41d0, { 0xaf, 0xed, 0xf2, 0x41, 0xe9, 0x74, 0xe0, 0x8e } }
+/// GUID for the DXE MM Ready To Lock protocol.
+pub const EFI_DXE_MM_READY_TO_LOCK_PROTOCOL_GUID: efi::Guid = efi::Guid::from_fields(


kuqin12 · 2026-03-17T04:57:31Z

patina_mm_user_core/src/lib.rs

+/// GUID for depex data HOBs paired with driver `MemoryAllocationModule` HOBs.
+///
+/// `gMmSupervisorDepexHobGuid`
+pub const MM_SUPERVISOR_DEPEX_HOB_GUID: efi::Guid = efi::Guid::from_fields(


kuqin12 · 2026-03-17T04:58:41Z

patina_mm_user_core/src/lib.rs

+}
+
+// SAFETY: MmUserCore is designed to be shared across threads with proper synchronization.
+unsafe impl Send for MmUserCore {}


This was inherited from supervisor module. Will remove.

kuqin12 · 2026-03-17T05:00:22Z

patina_mm_user_core/src/protocol_db.rs

@@ -0,0 +1,92 @@
+//! Protocol/Handle Database


:) that's what I thought as well. then the tpl based lock got in the way and I took the shorter route. Like the allocator comment, we will do that once this stabilizes.

kuqin12 · 2026-03-17T05:01:24Z

sdk/patina/src/timer/mod.rs

@@ -0,0 +1,135 @@
+//! Arch-specific timer functionality


Will create a separate PR for this for better readability.

kuqin12 · 2026-04-06T23:17:56Z

components/patina_mm_policy/Cargo.toml

This is not a component...

kuqin12 · 2026-04-06T23:19:02Z

core/patina_internal_mm_common/README.md

+including:
+
+- `UserCommandType` — Supervisor-to-user command enumeration
+- `MM_COMM_BUFFER_HOB_GUID` — Shared GUID for the communication buffer HOB


Stale doc. And rename the crate...

kuqin12 · 2026-04-06T23:19:22Z

patina_mm_supervisor_core/src/privilege_mgmt/call_gate.rs

+/// A call gate allows privilege level transitions through a far call instruction.
+#[repr(C, packed)]
+#[derive(Debug, Clone, Copy, Default)]
+pub struct CallGateDescriptor {


Is this really needed?

kuqin12 · 2026-04-06T23:19:31Z

patina_mm_supervisor_core/src/privilege_mgmt/mod.rs

kuqin12 · 2026-04-06T23:19:46Z

patina_mm_supervisor_core/src/privilege_mgmt/syscall_setup.rs

+//!   GS base addresses, allowing access to per-CPU data in the syscall handler.
+//!
+
+#![allow(unsafe_op_in_unsafe_fn)]


kuqin12 · 2026-04-06T23:26:25Z

patina_mm_supervisor_core/src/lib.rs

+// { 0x3efafe72, 0x3dbf, 0x4341, { 0xad, 0x04, 0x1c, 0xb6, 0xe8, 0xb6, 0x8e, 0x5e }}
+/// GUID used in MemoryAllocationModule HOBs to identify MM Supervisor module allocations.
+pub const MM_SUPERVISOR_HOB_MEMORY_ALLOC_MODULE_GUID: efi::Guid =
+    efi::Guid::from_fields(0x3efafe72, 0x3dbf, 0x4341, 0xad, 0x04, &[0x1c, 0xb6, 0xe8, 0xb6, 0x8e, 0x5e]);


Update this.

kuqin12 · 2026-04-06T23:27:00Z

patina_mm_user_core/src/lib.rs

+///
+/// `gMmSupervisorHobMemoryAllocModuleGuid`
+pub const MM_SUPERVISOR_HOB_MEMORY_ALLOC_MODULE_GUID: efi::Guid =
+    efi::Guid::from_fields(0x3efafe72, 0x3dbf, 0x4341, 0xad, 0x04, &[0x1c, 0xb6, 0xe8, 0xb6, 0x8e, 0x5e]);


Update this

kuqin12 · 2026-04-06T23:27:28Z

patina_mm_supervisor_core/src/mm_mem.rs

+
+// GUID for gEfiSmmSmramMemoryGuid
+// { 0x6dadf1d1, 0xd4cc, 0x4910, { 0xbb, 0x6e, 0x82, 0xb1, 0xfd, 0x80, 0xff, 0x3d }}
+pub const SMM_SMRAM_MEMORY_GUID: efi::Guid =


Update this

kuqin12 · 2026-04-06T23:28:40Z

patina_mm_supervisor_core/src/supervisor_handlers.rs

+// GUID for gEfiDxeMmReadyToLockProtocolGuid
+// { 0x60ff8964, 0xe906, 0x41d0, { 0xaf, 0xed, 0xf2, 0x41, 0xe9, 0x74, 0xe0, 0x8e } }
+/// GUID for the DXE MM Ready To Lock protocol.
+pub const EFI_DXE_MM_READY_TO_LOCK_PROTOCOL_GUID: efi::Guid =


kuqin12 · 2026-04-06T23:29:05Z

patina_mm_user_core/src/lib.rs

+/// GUID used in `MemoryAllocationModule` HOBs to identify MM Supervisor module allocations.
+///
+/// `gMmSupervisorHobMemoryAllocModuleGuid`
+pub const MM_SUPERVISOR_HOB_MEMORY_ALLOC_MODULE_GUID: efi::Guid =


github-actions bot added the impact:security Has a security impact label Mar 11, 2026

kuqin12 marked this pull request as draft March 11, 2026 00:33

kuqin12 changed the base branch from main to feature/supv March 11, 2026 01:15

kuqin12 force-pushed the supv_init branch from 7417dcd to c4b9fa9 Compare March 11, 2026 09:51